-
Notifications
You must be signed in to change notification settings - Fork 13.3k
[Coverage] Make additional counters available for BranchRegion. NFC. #112730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Coverage] Make additional counters available for BranchRegion. NFC. #112730
Conversation
`getBranchCounterPair()` allocates an additional Counter to SkipPath in `SingleByteCoverage`. `IsCounterEqual()` calculates the comparison with rewinding counter replacements. `NumRegionCounters` is updated to take additional counters in account. `incrementProfileCounter()` has a few additiona arguments. - `UseSkipPath=true`, to specify setting counters for SkipPath. It assumes `UseSkipPath=false` is used together. - `UseBoth` may be specified for marking another path. It introduces the same effect as issueing `markStmtAsUsed(!SkipPath, S)`. `llvm-cov` discovers counters in `FalseCount` to allocate `MaxCounterID` for empty profile data.
@llvm/pr-subscribers-pgo @llvm/pr-subscribers-clang Author: NAKAMURA Takumi (chapuni) Changes
Full diff: https://github.com/llvm/llvm-project/pull/112730.diff 5 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 89ac3b342d0a7c..cb1192bf6e11fe 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -1629,11 +1629,17 @@ class CodeGenFunction : public CodeGenTypeCache {
/// Increment the profiler's counter for the given statement by \p StepV.
/// If \p StepV is null, the default increment is 1.
void incrementProfileCounter(const Stmt *S, llvm::Value *StepV = nullptr) {
+ incrementProfileCounter(false, S, false, StepV);
+ }
+
+ void incrementProfileCounter(bool UseSkipPath, const Stmt *S,
+ bool UseBoth = false,
+ llvm::Value *StepV = nullptr) {
if (CGM.getCodeGenOpts().hasProfileClangInstr() &&
!CurFn->hasFnAttribute(llvm::Attribute::NoProfile) &&
!CurFn->hasFnAttribute(llvm::Attribute::SkipProfile)) {
auto AL = ApplyDebugLocation::CreateArtificial(*this);
- PGO.emitCounterSetOrIncrement(Builder, S, StepV);
+ PGO.emitCounterSetOrIncrement(Builder, S, UseSkipPath, UseBoth, StepV);
}
PGO.setCurrentStmt(S);
}
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 069469e3de856b..aefd53e12088b4 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -1138,6 +1138,19 @@ void CodeGenPGO::emitCounterRegionMapping(const Decl *D) {
if (CoverageMapping.empty())
return;
+ // Scan max(FalseCnt) and update NumRegionCounters.
+ unsigned MaxNumCounters = NumRegionCounters;
+ for (const auto [_, V] : *RegionCounterMap) {
+ auto HasCounters = V.getIsCounterPair();
+ assert((!HasCounters.first ||
+ MaxNumCounters > (V.first & CounterPair::Mask)) &&
+ "TrueCnt should not be reassigned");
+ if (HasCounters.second)
+ MaxNumCounters =
+ std::max(MaxNumCounters, (V.second & CounterPair::Mask) + 1);
+ }
+ NumRegionCounters = MaxNumCounters;
+
CGM.getCoverageMapping()->addFunctionMappingRecord(
FuncNameVar, FuncName, FunctionHash, CoverageMapping);
}
@@ -1193,11 +1206,25 @@ std::pair<bool, bool> CodeGenPGO::getIsCounterPair(const Stmt *S) const {
}
void CodeGenPGO::emitCounterSetOrIncrement(CGBuilderTy &Builder, const Stmt *S,
+ bool UseSkipPath, bool UseBoth,
llvm::Value *StepV) {
- if (!RegionCounterMap || !Builder.GetInsertBlock())
+ if (!RegionCounterMap)
return;
- unsigned Counter = (*RegionCounterMap)[S].first;
+ unsigned Counter;
+ auto &TheMap = (*RegionCounterMap)[S];
+ auto IsCounter = TheMap.getIsCounterPair();
+ if (!UseSkipPath) {
+ assert(IsCounter.first);
+ Counter = (TheMap.first & CounterPair::Mask);
+ } else {
+ if (!IsCounter.second)
+ return;
+ Counter = (TheMap.second & CounterPair::Mask);
+ }
+
+ if (!Builder.GetInsertBlock())
+ return;
// Make sure that pointer to global is passed in with zero addrspace
// This is relevant during GPU profiling
diff --git a/clang/lib/CodeGen/CodeGenPGO.h b/clang/lib/CodeGen/CodeGenPGO.h
index 83f35785e5327d..8b769dd88d7f1e 100644
--- a/clang/lib/CodeGen/CodeGenPGO.h
+++ b/clang/lib/CodeGen/CodeGenPGO.h
@@ -112,6 +112,7 @@ class CodeGenPGO {
public:
std::pair<bool, bool> getIsCounterPair(const Stmt *S) const;
void emitCounterSetOrIncrement(CGBuilderTy &Builder, const Stmt *S,
+ bool UseFalsePath, bool UseBoth,
llvm::Value *StepV);
void emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder, const Expr *S,
Address MCDCCondBitmapAddr,
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index a5d83e7a743bbd..0bcbd20593ae22 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -887,6 +887,9 @@ struct CounterCoverageMappingBuilder
/// The map of statements to count values.
llvm::DenseMap<const Stmt *, CounterPair> &CounterMap;
+ CounterExpressionBuilder::ReplaceMap MapToExpand;
+ unsigned NextCounterNum;
+
MCDC::State &MCDCState;
/// A stack of currently live regions.
@@ -922,15 +925,11 @@ struct CounterCoverageMappingBuilder
/// Return a counter for the sum of \c LHS and \c RHS.
Counter addCounters(Counter LHS, Counter RHS, bool Simplify = true) {
- assert(!llvm::EnableSingleByteCoverage &&
- "cannot add counters when single byte coverage mode is enabled");
return Builder.add(LHS, RHS, Simplify);
}
Counter addCounters(Counter C1, Counter C2, Counter C3,
bool Simplify = true) {
- assert(!llvm::EnableSingleByteCoverage &&
- "cannot add counters when single byte coverage mode is enabled");
return addCounters(addCounters(C1, C2, Simplify), C3, Simplify);
}
@@ -943,14 +942,31 @@ struct CounterCoverageMappingBuilder
std::pair<Counter, Counter> getBranchCounterPair(const Stmt *S,
Counter ParentCnt) {
- Counter ExecCnt = getRegionCounter(S);
- return {ExecCnt, Builder.subtract(ParentCnt, ExecCnt)};
+ auto &TheMap = CounterMap[S];
+ auto ExecCnt = Counter::getCounter(TheMap.first);
+ auto SkipExpr = Builder.subtract(ParentCnt, ExecCnt);
+
+ if (!llvm::EnableSingleByteCoverage)
+ return {ExecCnt, SkipExpr};
+
+ // Assign second if second is not assigned yet.
+ if (!TheMap.getIsCounterPair().second)
+ TheMap.second = NextCounterNum++;
+
+ Counter SkipCnt = Counter::getCounter(TheMap.second);
+ MapToExpand[SkipCnt] = SkipExpr;
+ return {ExecCnt, SkipCnt};
}
bool IsCounterEqual(Counter OutCount, Counter ParentCount) {
if (OutCount == ParentCount)
return true;
+ // Try comaparison with pre-replaced expressions.
+ if (Builder.replace(Builder.subtract(OutCount, ParentCount), MapToExpand)
+ .isZero())
+ return true;
+
return false;
}
@@ -1437,7 +1453,8 @@ struct CounterCoverageMappingBuilder
llvm::DenseMap<const Stmt *, CounterPair> &CounterMap,
MCDC::State &MCDCState, SourceManager &SM, const LangOptions &LangOpts)
: CoverageMappingBuilder(CVM, SM, LangOpts), CounterMap(CounterMap),
- MCDCState(MCDCState), MCDCBuilder(CVM.getCodeGenModule(), MCDCState) {}
+ NextCounterNum(CounterMap.size()), MCDCState(MCDCState),
+ MCDCBuilder(CVM.getCodeGenModule(), MCDCState) {}
/// Write the mapping data to the output stream
void write(llvm::raw_ostream &OS) {
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index b50f025d261e13..fc7f36c8599f54 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -640,6 +640,10 @@ static unsigned getMaxCounterID(const CounterMappingContext &Ctx,
unsigned MaxCounterID = 0;
for (const auto &Region : Record.MappingRegions) {
MaxCounterID = std::max(MaxCounterID, Ctx.getMaxCounterID(Region.Count));
+ if (Region.Kind == CounterMappingRegion::BranchRegion ||
+ Region.Kind == CounterMappingRegion::MCDCBranchRegion)
+ MaxCounterID =
+ std::max(MaxCounterID, Ctx.getMaxCounterID(Region.FalseCount));
}
return MaxCounterID;
}
|
@llvm/pr-subscribers-clang-codegen Author: NAKAMURA Takumi (chapuni) Changes
Full diff: https://github.com/llvm/llvm-project/pull/112730.diff 5 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 89ac3b342d0a7c..cb1192bf6e11fe 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -1629,11 +1629,17 @@ class CodeGenFunction : public CodeGenTypeCache {
/// Increment the profiler's counter for the given statement by \p StepV.
/// If \p StepV is null, the default increment is 1.
void incrementProfileCounter(const Stmt *S, llvm::Value *StepV = nullptr) {
+ incrementProfileCounter(false, S, false, StepV);
+ }
+
+ void incrementProfileCounter(bool UseSkipPath, const Stmt *S,
+ bool UseBoth = false,
+ llvm::Value *StepV = nullptr) {
if (CGM.getCodeGenOpts().hasProfileClangInstr() &&
!CurFn->hasFnAttribute(llvm::Attribute::NoProfile) &&
!CurFn->hasFnAttribute(llvm::Attribute::SkipProfile)) {
auto AL = ApplyDebugLocation::CreateArtificial(*this);
- PGO.emitCounterSetOrIncrement(Builder, S, StepV);
+ PGO.emitCounterSetOrIncrement(Builder, S, UseSkipPath, UseBoth, StepV);
}
PGO.setCurrentStmt(S);
}
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 069469e3de856b..aefd53e12088b4 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -1138,6 +1138,19 @@ void CodeGenPGO::emitCounterRegionMapping(const Decl *D) {
if (CoverageMapping.empty())
return;
+ // Scan max(FalseCnt) and update NumRegionCounters.
+ unsigned MaxNumCounters = NumRegionCounters;
+ for (const auto [_, V] : *RegionCounterMap) {
+ auto HasCounters = V.getIsCounterPair();
+ assert((!HasCounters.first ||
+ MaxNumCounters > (V.first & CounterPair::Mask)) &&
+ "TrueCnt should not be reassigned");
+ if (HasCounters.second)
+ MaxNumCounters =
+ std::max(MaxNumCounters, (V.second & CounterPair::Mask) + 1);
+ }
+ NumRegionCounters = MaxNumCounters;
+
CGM.getCoverageMapping()->addFunctionMappingRecord(
FuncNameVar, FuncName, FunctionHash, CoverageMapping);
}
@@ -1193,11 +1206,25 @@ std::pair<bool, bool> CodeGenPGO::getIsCounterPair(const Stmt *S) const {
}
void CodeGenPGO::emitCounterSetOrIncrement(CGBuilderTy &Builder, const Stmt *S,
+ bool UseSkipPath, bool UseBoth,
llvm::Value *StepV) {
- if (!RegionCounterMap || !Builder.GetInsertBlock())
+ if (!RegionCounterMap)
return;
- unsigned Counter = (*RegionCounterMap)[S].first;
+ unsigned Counter;
+ auto &TheMap = (*RegionCounterMap)[S];
+ auto IsCounter = TheMap.getIsCounterPair();
+ if (!UseSkipPath) {
+ assert(IsCounter.first);
+ Counter = (TheMap.first & CounterPair::Mask);
+ } else {
+ if (!IsCounter.second)
+ return;
+ Counter = (TheMap.second & CounterPair::Mask);
+ }
+
+ if (!Builder.GetInsertBlock())
+ return;
// Make sure that pointer to global is passed in with zero addrspace
// This is relevant during GPU profiling
diff --git a/clang/lib/CodeGen/CodeGenPGO.h b/clang/lib/CodeGen/CodeGenPGO.h
index 83f35785e5327d..8b769dd88d7f1e 100644
--- a/clang/lib/CodeGen/CodeGenPGO.h
+++ b/clang/lib/CodeGen/CodeGenPGO.h
@@ -112,6 +112,7 @@ class CodeGenPGO {
public:
std::pair<bool, bool> getIsCounterPair(const Stmt *S) const;
void emitCounterSetOrIncrement(CGBuilderTy &Builder, const Stmt *S,
+ bool UseFalsePath, bool UseBoth,
llvm::Value *StepV);
void emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder, const Expr *S,
Address MCDCCondBitmapAddr,
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index a5d83e7a743bbd..0bcbd20593ae22 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -887,6 +887,9 @@ struct CounterCoverageMappingBuilder
/// The map of statements to count values.
llvm::DenseMap<const Stmt *, CounterPair> &CounterMap;
+ CounterExpressionBuilder::ReplaceMap MapToExpand;
+ unsigned NextCounterNum;
+
MCDC::State &MCDCState;
/// A stack of currently live regions.
@@ -922,15 +925,11 @@ struct CounterCoverageMappingBuilder
/// Return a counter for the sum of \c LHS and \c RHS.
Counter addCounters(Counter LHS, Counter RHS, bool Simplify = true) {
- assert(!llvm::EnableSingleByteCoverage &&
- "cannot add counters when single byte coverage mode is enabled");
return Builder.add(LHS, RHS, Simplify);
}
Counter addCounters(Counter C1, Counter C2, Counter C3,
bool Simplify = true) {
- assert(!llvm::EnableSingleByteCoverage &&
- "cannot add counters when single byte coverage mode is enabled");
return addCounters(addCounters(C1, C2, Simplify), C3, Simplify);
}
@@ -943,14 +942,31 @@ struct CounterCoverageMappingBuilder
std::pair<Counter, Counter> getBranchCounterPair(const Stmt *S,
Counter ParentCnt) {
- Counter ExecCnt = getRegionCounter(S);
- return {ExecCnt, Builder.subtract(ParentCnt, ExecCnt)};
+ auto &TheMap = CounterMap[S];
+ auto ExecCnt = Counter::getCounter(TheMap.first);
+ auto SkipExpr = Builder.subtract(ParentCnt, ExecCnt);
+
+ if (!llvm::EnableSingleByteCoverage)
+ return {ExecCnt, SkipExpr};
+
+ // Assign second if second is not assigned yet.
+ if (!TheMap.getIsCounterPair().second)
+ TheMap.second = NextCounterNum++;
+
+ Counter SkipCnt = Counter::getCounter(TheMap.second);
+ MapToExpand[SkipCnt] = SkipExpr;
+ return {ExecCnt, SkipCnt};
}
bool IsCounterEqual(Counter OutCount, Counter ParentCount) {
if (OutCount == ParentCount)
return true;
+ // Try comaparison with pre-replaced expressions.
+ if (Builder.replace(Builder.subtract(OutCount, ParentCount), MapToExpand)
+ .isZero())
+ return true;
+
return false;
}
@@ -1437,7 +1453,8 @@ struct CounterCoverageMappingBuilder
llvm::DenseMap<const Stmt *, CounterPair> &CounterMap,
MCDC::State &MCDCState, SourceManager &SM, const LangOptions &LangOpts)
: CoverageMappingBuilder(CVM, SM, LangOpts), CounterMap(CounterMap),
- MCDCState(MCDCState), MCDCBuilder(CVM.getCodeGenModule(), MCDCState) {}
+ NextCounterNum(CounterMap.size()), MCDCState(MCDCState),
+ MCDCBuilder(CVM.getCodeGenModule(), MCDCState) {}
/// Write the mapping data to the output stream
void write(llvm::raw_ostream &OS) {
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index b50f025d261e13..fc7f36c8599f54 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -640,6 +640,10 @@ static unsigned getMaxCounterID(const CounterMappingContext &Ctx,
unsigned MaxCounterID = 0;
for (const auto &Region : Record.MappingRegions) {
MaxCounterID = std::max(MaxCounterID, Ctx.getMaxCounterID(Region.Count));
+ if (Region.Kind == CounterMappingRegion::BranchRegion ||
+ Region.Kind == CounterMappingRegion::MCDCBranchRegion)
+ MaxCounterID =
+ std::max(MaxCounterID, Ctx.getMaxCounterID(Region.FalseCount));
}
return MaxCounterID;
}
|
…puni/cov/single/nextcount
Currently `first` is not None by default.
return; | ||
|
||
unsigned Counter = (*RegionCounterMap)[S].first; | ||
unsigned Counter; | ||
auto &TheMap = (*RegionCounterMap)[S]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static_cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mention the use of auto
?
if (!UseSkipPath) { | ||
if (!IsCounter.first) | ||
return; | ||
Counter = (TheMap.first & CounterPair::Mask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch introduces a lot of code like
(X & CounterPair::Mask)
Would it make sense to create a small helper function to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I encapsulated it. See #112724.
Counter = (TheMap.second & CounterPair::Mask); | ||
} | ||
|
||
if (!Builder.GetInsertBlock()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When can this happen? Comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was the result of splitting the condition.
if (!RegionCounterMap || !Builder.GetInsertBlock())
return;
to
if (!RegionCounterMap)
return;
auto &TheMap = (*RegionCounterMap)[S]; // S should be allocated (as an initial value)
...
if (!Builder.GetInsertBlock())
return;
@@ -112,6 +112,7 @@ class CodeGenPGO { | |||
public: | |||
std::pair<bool, bool> getIsCounterPair(const Stmt *S) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function could use some documentation, or a better name.
This can happen in a later commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't name better.
return {I->second.Executed.hasValue(), I->second.Skipped.hasValue()};
It is defined in #112724.
} | ||
|
||
bool IsCounterEqual(Counter OutCount, Counter ParentCount) { | ||
if (OutCount == ParentCount) | ||
return true; | ||
|
||
// Try comaparison with pre-replaced expressions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put a small example in the comment here?
@@ -638,6 +638,10 @@ static unsigned getMaxCounterID(const CounterMappingContext &Ctx, | |||
unsigned MaxCounterID = 0; | |||
for (const auto &Region : Record.MappingRegions) { | |||
MaxCounterID = std::max(MaxCounterID, Ctx.getMaxCounterID(Region.Count)); | |||
if (Region.Kind == CounterMappingRegion::BranchRegion || | |||
Region.Kind == CounterMappingRegion::MCDCBranchRegion) | |||
MaxCounterID = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a updateMaxCounterID
helper? I think this happens in a couple places in the code now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there was no idea in initial designs to increase Counter
s dynamically. It shall be improved in the future.
…puni/cov/single/nextcount Conflicts: clang/lib/CodeGen/CoverageMappingGen.cpp
Conflicts: clang/lib/CodeGen/CoverageMappingGen.cpp llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
c0a93e2
into
users/chapuni/cov/single/nextcount-base
I've accidentally closed this with pushing the wrong base. Please move to the re-created #120930 . |
getBranchCounterPair()
allocates an additional Counter to SkipPath inSingleByteCoverage
.IsCounterEqual()
calculates the comparison with rewinding counter replacements.NumRegionCounters
is updated to take additional counters in account.incrementProfileCounter()
has a few additiona arguments.UseSkipPath=true
, to specify setting counters for SkipPath. It assumesUseSkipPath=false
is used together.UseBoth
may be specified for marking another path. It introduces the same effect as issueingmarkStmtAsUsed(!SkipPath, S)
.llvm-cov
discovers counters inFalseCount
to allocateMaxCounterID
for empty profile data.https://discourse.llvm.org/t/rfc-integrating-singlebytecoverage-with-branch-coverage/82492
Depends on: #112698 #112702 #112724